Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send message to DLQ after correct number of attempts #325

Merged

Conversation

Thomasvdam
Copy link
Contributor

I just noticed you released the JSON API update, congratulations and thanks for your time and effort!

I was trying it out today and everything seems to work as expected, except for DLQ MaxReceiveCounts. As described in #263 in AWS the attribute describes the number of attempts before sending a message to the DLQ, not the number of retries.

There was also a small error when iterating over the messages where when a message that got sent to the DLQ would skip the next message in that queue for the timeout check in that iteration.

Closes: #263

@Thomasvdam Thomasvdam force-pushed the chore/message-timeout branch from 95c4ab3 to 6063fc1 Compare October 3, 2024 13:03
Copy link
Owner

@Admiral-Piett Admiral-Piett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me so long to get to this! It's been a hectic time of year. Regardless, thanks for pitching in on this issue. I have one minor question there then the code looks good. Could you throw in a test for it as well? Then I think it's all set.

app/gosqs/change_message_visibility.go Outdated Show resolved Hide resolved
@Thomasvdam Thomasvdam force-pushed the chore/message-timeout branch from 6063fc1 to a39305c Compare October 22, 2024 15:47
@Thomasvdam
Copy link
Contributor Author

Sorry it took me so long to get to this! It's been a hectic time of year. Regardless, thanks for pitching in on this issue. I have one minor question there then the code looks good. Could you throw in a test for it as well? Then I think it's all set.

No problem at all! Open Source should be fun so please don't feel pressured :)

I tweaked the existing test to make it fail without the fix and added a new test to test the loop counter adjustment in app/gosqs/gosqs_test.go. I'm not sure what your preference is in tests in terms of duplicate code v.s. maintaining abstractions in the test files. If you prefer me to move some shared code to a helper just let me know.

@Admiral-Piett
Copy link
Owner

@Thomasvdam This looks awesome, thanks for sticking with it for so long! This looks great to me! Though I see that I probably burned you by moving some things around, would you mind rebasing? I assume that would fix your tests there.

Otherwise, I'm ready to do the merge deed! Are you anxiously awaited this? I had to get some fixes into the 0.5.1 release, which just went out, so I missed sneaking you into that, but I could make this 0.5.2 if you're needing it, like, yesterday?

@Thomasvdam
Copy link
Contributor Author

No rush from my end. I'm using my fork so please release at your own pace. I'll try the rebase now

@Thomasvdam Thomasvdam force-pushed the chore/message-timeout branch from a39305c to 5bca14b Compare November 1, 2024 14:22
@Thomasvdam
Copy link
Contributor Author

Ah I didn't know about the --race flag, I'll have to look into that!

@Admiral-Piett
Copy link
Owner

Ah I didn't know about the --race flag, I'll have to look into that!

Ah yeah, that gets me too. Let me know if you get stuck after trying it with that and I'll have a look and see if I can shake anything loose. Thank you!

@Thomasvdam Thomasvdam force-pushed the chore/message-timeout branch from 5bca14b to cda9b09 Compare November 3, 2024 17:36
@Thomasvdam
Copy link
Contributor Author

I think this should get the tests to pass reliably. It feels a little hacky though, if you have pointers on a proper way to do this I'd love to learn :)

@Admiral-Piett
Copy link
Owner

@Thomasvdam Yeah, I think to rework it, which is worthy, would take some significant refactoring. What you've got is good, since we were never calling done before and therefor probably never shutting down out multiple goroutines. So thanks for taking care of that. Looks good to me, merging!

@Admiral-Piett Admiral-Piett merged commit b7de554 into Admiral-Piett:master Nov 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redrive policy MaxReceive logic is off by one
2 participants